@clawsweeper review

🦞👀
ClawSweeper picked this up.

Command router queued. I will update this comment with the next step.

Codex review: needs maintainer review before merge. Reviewed August 2, 2026, 1:44 AM ET / 05:44 UTC.

ClawSweeper review

What this changes

The branch makes a Messages bridge request fail promptly when a relaunch clears its queued file, while treating every vanished request without a response as delivery-unknown rather than retry-safe.

Merge readiness

⚠️ Ready for maintainer review - 2 items remain

This PR remains necessary: current main still waits only for a response file and can hold a discarded bridge request until its full timeout. The repaired head preserves the existing timeout error for every vanished request without a reply, resolving the earlier unsafe retry classification; no actionable patch finding remains.

Priority: P1
Reviewed head: 5c63215b1d5f3dcc920a160ef8ea568995e61910

Review scores

Verification

How this fits together

The iMessage bridge writes each CLI or gateway operation into a Messages-container inbox; the injected Messages helper claims it, performs the action, and writes a response into an outbox. The client poller returns that response to the caller, but a Messages relaunch can clear both queue directories while a request may be in flight.

flowchart LR
  A[CLI or gateway operation] --> B[Bridge client writes request]
  B --> C[Messages bridge inbox]
  C --> D[Injected Messages helper]
  D --> E[Response outbox]
  E --> F[Caller result]
  G[Messages relaunch] --> H[Queue directories cleared]
  H --> F

Before merge

• Resolve merge risk (P2) - The timeout error can now be returned after one polling interval rather than after the configured full timeout when a request vanishes; this preserves its delivery-unknown meaning but changes timing for callers that had implicitly relied on waiting longer.
• Complete next step (P2) - No focused automated repair remains; the contributor or maintainer only needs to clear the draft state and land the already-reviewed current head.

@clawsweeper re-review

🦞👀
ClawSweeper picked this up.

Command router queued. I will update this comment with the next step.

Thanks — the finding pointed at a real problem, though not via the mechanism described. Fixed in 1f87ec9.

On the stated ordering. The review says the producer "removes its processing claim and writes its reply as separate steps, leaving an interval where the action may already have run but the reply is not yet visible." The order is the opposite. In processV2InboxFile the reply is renamed into the outbox before the claim is dropped:

response = processV2Envelope(envelope);      // action executes
...
[responseData writeToFile:tmp atomically:NO];
rename(tmp.UTF8String, outPath.UTF8String);  // reply published (atomic)

// Drop the claimed request — we're done with it. If the process dies
// after claiming, a later inbox scan removes the orphan without
// replaying a potentially delivered side effect.
[[NSFileManager defaultManager] removeItemAtPath:claimPath error:nil];

So "keep the processing claim until the reply is published" is already the invariant, and reordering would be a no-op.

But the concern is valid through another path, which that comment names: if the dylib dies after IMCore delivered and before the reply is written, the claim is orphaned, and cleanupOrphanedV2Claims or a relaunch clears it. My check then saw nothing and reported .bridgeNotReady, which this PR documents as retry-safe — for a message that may well have gone out. On the machine this was found on, Messages.app dies every 2-3 days, so that window is not hypothetical.

The defect was my error semantics, not the producer ordering: I collapsed two different situations into one retry-safe error.

Fix. Track whether the dylib ever claimed the request:

requestStillQueued became requestQueueState returning unclaimed / claimed / absent. An inbox that cannot be enumerated still reports unclaimed, so a transient read error never ends a live request.

Added the interleaving test that was asked for: one request walked through unclaimed → absent, another through unclaimed → claimed → absent, plus a guard that the two error cases cannot be conflated.

swift test: 491 tests in 4 suites passing. swift format lint and swiftlint clean on all three touched files; git diff --check clean.

Two things I did not change, deliberately:

• No producer-side reordering, since the invariant already holds. If you would still prefer a durable producer-side record (e.g. keeping the claim until the reply is confirmed readable), say so and I will do it — but it looked like added protocol surface for an invariant that is already satisfied.
• The residual window is narrowed, not closed. A crash between delivery and reply publication is still unobservable from the client; the change makes that case report unknown rather than retry-safe, which is the honest outcome rather than a guarantee.

@clawsweeper re-review

🦞👀
ClawSweeper picked this up.

Command router queued. I will update this comment with the next step.

@clawsweeper re-review

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: when the review finishes, ClawSweeper will create the durable review comment if needed or update the existing comment in place.

Re-review progress:

• State: Review in progress
• Detail: The exact-review queue leased this run; Codex is reviewing the item.
• Run: https://github.com/openclaw/clawsweeper/actions/runs/30536272159
• Updated: 2026-07-30T10:51:43.425Z

Note for anyone reading this PR: the review shown above is stale. It was produced against 94b5811 and predates the fix for its own P1.

The P1 ("Keep the processing claim until the reply is published") was addressed in 1f87ec9, along with the interleaving test it asked for. Summary of that exchange, in case the bot verdict never refreshes:

• The finding's stated mechanism is inverted. processV2InboxFile renames the reply into the outbox before dropping the claim, so the requested ordering is already the invariant and reordering would be a no-op. Quoted source is in the reply above.
• The underlying concern is real via a different path, which that function's own comment names: if the dylib dies after IMCore delivered but before the reply is written, the claim is orphaned and later cleared. My original code reported that as retry-safe .bridgeNotReady.
• Fixed by splitting the error semantics rather than the ordering: never-claimed-then-absent stays .bridgeNotReady (retry-safe); claimed-then-absent-with-no-reply becomes the new .deliveryUnknown(action:), which must not be retried blind.

State at 1f87ec9: swift test 491 tests in 4 suites passing, swift format lint and swiftlint clean on all touched files, git diff --check clean. Proof of the original stall fix was accepted in the last review (proof: sufficient).

Bot-side observation, in case it is useful to whoever maintains ClawSweeper: re-review requests are acking within seconds but not producing a review, and it only started after the head moved. Reviews ran fine at 94b5811 (ack 06:32 → verdict 06:38; ack 14:50 → verdict 15:23), and all three requests since the push to 1f87ec9 have acked and gone quiet (19:08, 22:15, plus the automatic trigger on the body update). Not re-issuing the command to avoid piling on a possibly-active lease.

Maintainer repair is on the branch at 5c63215; I recommend landing this PR once CI is green.

The fast-failure mechanism is sound, but the original branch inferred retry safety from whether this client happened to observe a .processing file. That state can be created, acted on, and removed entirely between two 50ms polls, so “never observed claimed” did not prove the send was safe to retry. The repair treats every vanished request without a reply as delivery-unknown via the existing timeout case, preserving source compatibility and preventing blind duplicate sends.

Proof on macOS arm64:

• Queue-state suite: all 5 tests passed, covering unclaimed, claimed, absent, unreadable, and both absent-state histories.
• make lint: passed with pre-existing warnings only.
• make test: all 490 tests passed after the known attachment-descendant PID-file flake failed once; its exact retry and the complete retry passed.
• make build ARCHES="$(uname -m)": release CLI and universal helper built successfully.
• Live release status run succeeded and reported 0.13.5, SIP enabled, v2_ready=false, and basic features available. That host state makes a second destructive Messages relaunch/fault-injection run inappropriate; the contributor’s real bridge restart reproduction on this PR remains the live proof of the fast-failure path. The maintainer repair changes only retry classification after that same disappearance.
• Local-diff and full-branch autoreviews: clean, no actionable findings.

No message was sent and Messages.app was not relaunched during maintainer verification.